-
Notifications
You must be signed in to change notification settings - Fork 325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AnyCPU tests to choose default architecture based on process #2206
AnyCPU tests to choose default architecture based on process #2206
Conversation
src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs
Outdated
Show resolved
Hide resolved
@@ -12,7 +12,7 @@ | |||
<Visible>False</Visible> | |||
</Content> | |||
</ItemGroup> | |||
<ItemGroup Condition=" '$(Platform)' != 'x86' AND '$(OS)' == 'Windows_NT'" > | |||
<ItemGroup Condition=" ('$(Platform)' == 'x64' OR '$(PlatformTarget)' == 'x64') AND '$(OS)' == 'Windows_NT'" > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OS [](start = 89, length = 2)
what happens when we give mulitple PlatformTargets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With multiple PlatformTargets, dotnet build generates AnyCPU dll (then picks testhost from nuget path depending upon default architecture)
[TestMethod] | ||
public void GetTestHostProcessStartInfoShouldUseTestHostX86ExeFromNugetIfNotFoundInSourceLocation() | ||
{ | ||
var testhostExePath = "testhost.x86.exe"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if you can reduce this test by mocking only the relevant call, I am not sure if your change is related to this.
This PR will probably also fix #2215 |
@vagisha-nidhi Are we done with all the changes here ? |
Yes. Please review this. |
@@ -0,0 +1,9 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<Project ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we try CopyToPublishDirectory in the props itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried. It works. Updated the PR
src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Default architecture to be decided whether the process is vstest.console or dotnet(32bit) or dotnet(64bit).
Also, getting testhost.exe path from nuget folder.
Changing the copying mechanism for testhost exes. We need to only copy test host exe in case of publish scenario (since already finding in nuget folders for build scenario)
Default architecture is set as follows:
a. if dotnet 32 bit : default architecture is x86
b. if dotnet 64 bit : default architecture is x64
The logic to find native executable testhost.exe is also modified.
For x64 and x86 dlls, the corresponding testhost/testhost.x86.exe is copied to the source folder.
For AnyCPU, there willbe no copying of testhost exe.
dotnet test --runtime win7-x86
anddotnet test --runtime win7-x64
generates corresponding x86 and x64 dllsRelated Issue
#2189